-
Notifications
You must be signed in to change notification settings - Fork 90
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enhance Base64FileField #149
Conversation
so one can get the content_type header from the base64field in models easily
…th requirements with python setup.py develop/install etc.
drf_extra_fields/fields.py
Outdated
DEFAULT_CONTENT_TYPE = "application/octet-stream" | ||
|
||
|
||
class Base64FieldMixin(object): | ||
trust_provided_content_type = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be provided via init method. So that you don't need your own class to change this.
Similar to represent_in_base64
usage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. Thank you.
@ybrs This is almost done as I promised you last year :)
setup.py
Outdated
version='3.0.4', | ||
version='3.0.5', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like changing ContentFile
to SimpleUploadedFile
is backward incompatible. ContentFile
overrides following methods: __init__
, __str__
, __bool__
, open
, close
and write
. These methods also exist in SimpleUploadedFile
but they are slightly different in two classes, so it may cause bugs.
Considering this, should we update the major version, or changing minor version is enough? I think changing patch version (like in this commit) is erroneous. What do you think? @alicertel
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's go with 3.1.0 and document this breaking change in the beginning of the readme. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank so much. I will take a new release soon.
Completed version of #84. Thanks @ybrs for the contribution.